Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Allow usage of m.always_on_screen capability by all widgets #5399

Closed
wants to merge 1 commit into from
Closed

Allow usage of m.always_on_screen capability by all widgets #5399

wants to merge 1 commit into from

Conversation

@alangecker
Copy link

@alangecker alangecker commented Nov 5, 2020

like metioned in element-hq/element-web#15001 the m.always_on_screen capability currently only works only for jitsi widgets.
A reason is provided as a commit in the source code:

Obviously anyone that can add a widget can claim it's a jitsi widget, so this doesn't really offer much over the set of domains we load widgets from at all, but it probably makes sense for sanity.

I think this currently doesn't provide any additional security or sanity, but blocks people from developing other persistent widgets.

In this PR I removed this restriction.

Open Challenge

merging it like it is now would lead to every single widget being wrapped in an PersistedElement.

❓ Anyone any idea how we can somehow retrieve at this place whether the m.always_on_screen capability got actually requested?

matrix-react-sdk/src/components/views/elements/AppTile.js

Lines 378 to 390 in cbd568a

// if the widget would be allowed to remain on screen, we must put it in
// a PersistedElement from the get-go, otherwise the iframe will be
// re-mounted later when we do.
if (this.props.whitelistCapabilities.includes('m.always_on_screen')) {
const PersistedElement = sdk.getComponent("elements.PersistedElement");
// Also wrap the PersistedElement in a div to fix the height, otherwise
// AppTile's border is in the wrong place
appTileBody = <div className="mx_AppTile_persistedWrapper">
<PersistedElement persistKey={this._persistKey}>
{appTileBody}
</PersistedElement>
</div>;
}

closes element-hq/element-web#15001

@t3chguy
Copy link
Member

t3chguy commented Nov 5, 2020

This will likely need a user prompt otherwise can be abused by a widget going transparent without you seeing it or other similar illicit things

@turt2live
Copy link
Member

turt2live commented Nov 5, 2020

Yea, this is something that will need to be considered by the widget rearchitecting. I'll take this under my wing to incorporate into the discussions that need to be had.

Thanks for the PR in any case!

@turt2live turt2live self-assigned this Nov 5, 2020
@turt2live
Copy link
Member

turt2live commented Nov 20, 2020

gah, I forgot about this PR, sorry :(

There's quite a few places where we made assumptions about sticky widgets always being jitsi widgets, surprisingly. I don't think it's an issue to have widgets be PersistedElements everywhere - they don't have a long lifetime if they don't get stuck to the screen anyhow. It's a little bit more work than just giving it the capability though.

@turt2live
Copy link
Member

turt2live commented Nov 20, 2020

Closing in favour of #5435 - thanks again for the PR, and apologies for completely forgetting this exists.

@turt2live turt2live closed this Nov 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@turt2live turt2live

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Widgets other than Jitsi requesting m.always_on_screen are not treated as persistent widgets

3 participants

@alangecker @t3chguy @turt2live